Skip to content

feature/RM-9546 update selenium to 4.35.0 #1366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

feature/RM-9546 update selenium to 4.35.0 #1366

wants to merge 7 commits into from

Conversation

Ottorium
Copy link
Contributor

No description provided.

MichaelKetting
MichaelKetting requested changes Aug 25, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something went sideways, the PR has the old messages and syntax, not the ones from the previous PR.

@Ottorium Ottorium force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from a1ef5d0 to 190a116 Compare August 25, 2025 14:38
@MichaelKetting
Copy link
Member

MichaelKetting commented Aug 28, 2025 •
edited by dkudernatsch
Loading

  • Resolve the Bidi Connection problem with Selenium Firefox

@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch 3 times, most recently from bb7010e to 709d9f1 Compare September 11, 2025 08:10
@dkudernatsch
Copy link
Contributor

dkudernatsch commented Sep 11, 2025 •
edited
Loading

  • Resolve the Bidi Connection problem with Selenium Firefox

The firefox problem seems to be resolved, my best guess is, that there were not disposed WebSocket connections.

@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch 2 times, most recently from 709d9f1 to 9a63aee Compare September 11, 2025 08:17
MichaelKetting
MichaelKetting requested changes Sep 11, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code looks okay, I got some questions about how things actually work together.

I do have a big request: now that we got a working solution, please split the bidi-stuff into it's own PR and jira ticket (bug?) and let's do this new ticket first. I don't want to hide the semantic changes in a simple Selenium upgrade ticket, in particular since it is not actually dependet on the update and might have been incorrect from the start.

@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch 4 times, most recently from 8629ca0 to 1c41ab8 Compare September 24, 2025 08:14
@MichaelKetting MichaelKetting force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from 044dfa0 to b493069 Compare September 29, 2025 06:53
@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch 2 times, most recently from d47800e to 9940570 Compare October 8, 2025 08:36
@dkudernatsch dkudernatsch self-assigned this Oct 8, 2025
MichaelKetting
MichaelKetting requested changes Oct 8, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge commits where the selenium version gest updated.

Some small design discussions we should desicde before this is done.

MichaelKetting
MichaelKetting requested changes Oct 14, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments in PR. We should at least decide to keep the status quo of the PR.

@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from 9940570 to 311bb86 Compare October 15, 2025 08:12
@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from 6874047 to 6f7ab7c Compare October 15, 2025 08:56
@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from 6f7ab7c to 2f0d992 Compare October 15, 2025 08:57
MichaelKetting
MichaelKetting reviewed Oct 15, 2025
View reviewed changes
Remotion/Web/Development.WebTesting/BrowserSession/IBidiConnectionProvider.cs
/// <summary>
/// Used for timeouts for opening connections and interactions with the browser
/// </summary>
TimeSpan DefaultBidiTimeout => TimeSpan.FromSeconds(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to use default interfce implementations right now.

Remotion/Web/Development.WebTesting/BrowserSession/IBidiConnectionProvider.cs
/// <summary>
/// Used for timeouts for opening connections and interactions with the browser
/// </summary>
TimeSpan DefaultBidiTimeout => TimeSpan.FromSeconds(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected this information is taken from the browser session.

Remotion/Web/Development.WebTesting/BrowserSession/IBrowserSession.cs
/// Represents a wrapper around a <see cref="Coypu.BrowserSession"/> which has additional cleanup routines via <see cref="IDisposable.Dispose"/>.
/// </summary>
public interface IBrowserSession : IDisposable
public interface IBrowserSession : IBidiConnectionProvider, IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected that the interface would be on BrowserSession, not on the IBrowserSession. I understand why after llooking in the code but it feels wrong. I just don't have a good answer yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we should be able to change the usage places to BrowserSessionBase or possibly a T that requires IBrowserSession and IBidiConnectionProvider. This would allow us to move IBdiConnectionProvider from the IBrowserSession interface to BrowserSessionBase. I'm not sure which version I would prefer, one's not using interfaces, the other might introduce additional dependencies (type constraints) in the future. Probably the interface version, though (gut feeling).

The change would affect FirefoxBrowserSession.ApplyDefaultWebTestFeatures and the other two BrowserSession implementations typed to browser.

MichaelKetting
MichaelKetting requested changes Oct 15, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the round trip, I think this should be straight forward now. 🤞

Remotion/Web/Development.WebTesting/BrowserSession/IBrowserSession.cs
/// Represents a wrapper around a <see cref="Coypu.BrowserSession"/> which has additional cleanup routines via <see cref="IDisposable.Dispose"/>.
/// </summary>
public interface IBrowserSession : IDisposable
public interface IBrowserSession : IBidiConnectionProvider, IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we should be able to change the usage places to BrowserSessionBase or possibly a T that requires IBrowserSession and IBidiConnectionProvider. This would allow us to move IBdiConnectionProvider from the IBrowserSession interface to BrowserSessionBase. I'm not sure which version I would prefer, one's not using interfaces, the other might introduce additional dependencies (type constraints) in the future. Probably the interface version, though (gut feeling).

The change would affect FirefoxBrowserSession.ApplyDefaultWebTestFeatures and the other two BrowserSession implementations typed to browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dkudernatsch dkudernatsch dkudernatsch left review comments

@MichaelKetting MichaelKetting MichaelKetting requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@dkudernatsch dkudernatsch

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Ottorium @MichaelKetting @dkudernatsch